Support Ruby 3.2, 3.3 and Rails 7.1 #3

ohbarye · 2024-05-26T08:23:46Z

Changes

My first motivation is to run tests against Ruby 3.2 & 3.3. But I found that this gem doesn't work with Rails 7.1, so I created some patches for that and added version combinations to the test matrix.

Thoughts

I wonder if it's okay to remove 2.6 to 3.0 support since they are already EOL.
https://endoflife.date/ruby

``` 1) ActiveRecord::DebugErrors::DisplayConnectionOwners#execute when ActiveRecord::Deadlocked occurs when the user has the permission to execute 'SHOW ENGINE INNODB STATUS' displays latest detected deadlock Failure/Error: ActiveRecord::Base.legacy_connection_handling = false NoMethodError: undefined method `legacy_connection_handling=' for class ActiveRecord::Base # ./vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.3.3/lib/active_record/dynamic_matchers.rb:22:in `method_missing' # ./spec/spec_helper.rb:29:in `block (2 levels) in <top (required)>' ```

ohbarye · 2024-05-26T08:35:14Z

Oops, I found the latest Rails breaks something... Let me check the CI failures.

…ecute` Starting with Rails 7.1, `raw_execute` may not be called when executing a query. This pull request introduces `with_raw_connection`, and this method is now called when executing a query. https://github.com/rails/rails/pull/44576/files#diff-460f4e7973c5dd945c51d24df5b0173961190d3645f4e2585fd3003fa1fc0ff7R865 Overriding this method and calling super will result in a LocalJumpError, which is not safe. In this commit, I tried to call the log by hooking into `translate_exception_class`. This method has been around since 4.2, and there should be no changes to the interface since 6.0. rails/rails@5e5118a#diff-460f4e7973c5dd945c51d24df5b0173961190d3645f4e2585fd3003fa1fc0ff7R356

`acquire_connection` in connection_pool is now smarter in Rails 7.1.3. Even when running existing tests in Rails7.1.3, `ActiveRecord::ConnectionTimeoutError` no longer occurs because a connection can be obtained by `try_to_checkout_new_connection` after reap. https://github.com/rails/rails/compare/v7.1.2..v7.1.3#diff-642b90553b888bd2c724c093a1a685a5408a7d8293f3751366c25dc548936eb7R660

ohbarye · 2024-05-26T11:39:18Z

Although the diff got larger than I expected, I was finally able to pass test for all supported versions: 6.0, 6,1, 7.0, 7.1.

See passed tests here: https://github.com/ohbarye/activerecord-debug_errors/actions/runs/9242922830

ohbarye · 2024-05-26T11:43:57Z

lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb

+          raise
+        end
+        private method_name if method_name == :raw_execute
+      elsif ActiveRecord.version >= Gem::Version.new("7.1.0")


As commented in 5f9a8c9, there is a case when neither raw_execute nor execute is called in Rails 7.1.

I'm not sure if this way is the best, I tried to use translate_exception_class as a hook for Rails 7.1.

I think the following approach is similar to the original one and more readable. What do you think about them?

diff --git a/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb b/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb index 77f3f25..680e2e6 100644 --- a/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb +++ b/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb @@ -80,6 +80,12 @@ module ActiveRecord end end +ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter.descendants.each do |adapter| + adapter.prepend(ActiveRecord::DebugErrors::DisplayMySQLInformation) +end class ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter - prepend ActiveRecord::DebugErrors::DisplayMySQLInformation + def self.inherited(base) + super + base.prepend(ActiveRecord::DebugErrors::DisplayMySQLInformation) + end end

I think the code you presented is quite neat!

I think, I'm probably misunderstanding something big (and my implementation has based on the misunderstanding), but could you please explain to me why your code still works in Rails 7.1 for further studies? (and why the original code doesn't work.)

As you can see below, ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter#raw_execute is not implemented, whereas the subclasses have the method raw_execute, so appending the module to the subclasses works expectedly:

% bundle exec irb irb(main):001> require "active_record" => true irb(main):002> require "active_record/connection_adapters/abstract_mysql_adapter" => true irb(main):003> require "active_record/connection_adapters/mysql2_adapter" => true irb(main):004> require "active_record/connection_adapters/trilogy_adapter" => true irb(main):005> show_source ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter#raw_execute From: /Users/arabiki/ghq/src/github.com/abicky/activerecord-debug_errors/gemfiles/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.3.3/lib/active_record/connection_adapters/abstract/database_statements.rb:530 def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) raise NotImplementedError end => nil irb(main):006> show_source ActiveRecord::ConnectionAdapters::Mysql2Adapter#raw_execute From: /Users/arabiki/ghq/src/github.com/abicky/activerecord-debug_errors/gemfiles/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.3.3/lib/active_record/connection_adapters/mysql2/database_statements.rb:96 def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) log(sql, name, async: async) do with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| sync_timezone_changes(conn) result = conn.query(sql) verified! handle_warnings(sql) result end end end => nil irb(main):007> show_source ActiveRecord::ConnectionAdapters::TrilogyAdapter#raw_execute From: /Users/arabiki/ghq/src/github.com/abicky/activerecord-debug_errors/gemfiles/vendor/bundle/ruby/3.3.0/gems/activerecord-7.1.3.3/lib/active_record/connection_adapters/trilogy/database_statements.rb:44 def raw_execute(sql, name, async: false, allow_retry: false, materialize_transactions: true) log(sql, name, async: async) do with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| sync_timezone_changes(conn) result = conn.query(sql) verified! handle_warnings(sql) result end end end => nil

Ah, I got it now. Thanks for your explanation!

ohbarye · 2024-05-26T11:45:08Z

spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb

      Thread.new { sleep 10 } # another thread

      expect {
        ActiveRecord::Base.connection # Ensure to acquire a connection
        Array.new(ActiveRecord::Base.connection_pool.size) do
          Thread.new do
-            ActiveRecord::Base.connection_pool.checkout(0.1)
+            ActiveRecord::Base.connection_pool.checkout
+            sleep 0.001


Connection pool handling in Rails 7.1 is so smarter that the existing test cannot cause ConnectionTimeoutError. To cause it, I tweaked the code.

ref: 1688bd0

I'm afraid that this test becomes flaky because some threads might end before the last thread calls checkout. How about the following changes?

diff --git a/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb b/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb index 7a86bc7..6f2b1fe 100644 --- a/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb +++ b/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb @@ -20,11 +20,20 @@ RSpec.describe ActiveRecord::DebugErrors::DisplayConnectionOwners do it "displays connection owners and other threads" do Thread.new { sleep 10 } # another thread + mutex = Mutex.new + cv = ConditionVariable.new + expect { ActiveRecord::Base.connection # Ensure to acquire a connection Array.new(ActiveRecord::Base.connection_pool.size) do Thread.new do - ActiveRecord::Base.connection_pool.checkout(0.1) + mutex.synchronize do + ActiveRecord::Base.connection_pool.checkout(0.1) + cv.wait(mutex, 1) + rescue + cv.broadcast + raise + end end end.each(&:join) }.to raise_error(ActiveRecord::ConnectionTimeoutError)

Exactly. And to be honest, I couldn't think of any other way to do it, so I'm glad to know the cool way you suggested it. Updated d2a4654

ohbarye · 2024-05-26T11:45:35Z

spec/spec_helper.rb

+    # For compatibility. Rails deprecated since 6.1 and removed this option since 7.1.
+    # https://github.com/rails/rails/pull/44827/commits/ad52c0a19714a1b87a7d0c626a8b364cf95414cf
+    if ActiveRecord::Base.respond_to?(:legacy_connection_handling)
+      ActiveRecord::Base.legacy_connection_handling = false


It is simply broken in Rails 7.1.

abicky

Thank you for your pull request! I appreciate your contribution.
I've checked the changes and made some comments.

abicky · 2024-05-26T13:34:06Z

lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb

+          raise
+        end
+        private method_name if method_name == :raw_execute
+      elsif ActiveRecord.version >= Gem::Version.new("7.1.0")


I think the following approach is similar to the original one and more readable. What do you think about them?

diff --git a/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb b/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb index 77f3f25..680e2e6 100644 --- a/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb +++ b/lib/activerecord/debug_errors/ext/connection_adapters/abstract_mysql_adapter.rb @@ -80,6 +80,12 @@ module ActiveRecord end end +ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter.descendants.each do |adapter| + adapter.prepend(ActiveRecord::DebugErrors::DisplayMySQLInformation) +end class ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter - prepend ActiveRecord::DebugErrors::DisplayMySQLInformation + def self.inherited(base) + super + base.prepend(ActiveRecord::DebugErrors::DisplayMySQLInformation) + end end

abicky · 2024-05-26T14:00:23Z

spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb

      Thread.new { sleep 10 } # another thread

      expect {
        ActiveRecord::Base.connection # Ensure to acquire a connection
        Array.new(ActiveRecord::Base.connection_pool.size) do
          Thread.new do
-            ActiveRecord::Base.connection_pool.checkout(0.1)
+            ActiveRecord::Base.connection_pool.checkout
+            sleep 0.001


I'm afraid that this test becomes flaky because some threads might end before the last thread calls checkout. How about the following changes?

diff --git a/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb b/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb index 7a86bc7..6f2b1fe 100644 --- a/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb +++ b/spec/activerecord/debug_errors/ext/connection_adapters/connection_pool_spec.rb @@ -20,11 +20,20 @@ RSpec.describe ActiveRecord::DebugErrors::DisplayConnectionOwners do it "displays connection owners and other threads" do Thread.new { sleep 10 } # another thread + mutex = Mutex.new + cv = ConditionVariable.new + expect { ActiveRecord::Base.connection # Ensure to acquire a connection Array.new(ActiveRecord::Base.connection_pool.size) do Thread.new do - ActiveRecord::Base.connection_pool.checkout(0.1) + mutex.synchronize do + ActiveRecord::Base.connection_pool.checkout(0.1) + cv.wait(mutex, 1) + rescue + cv.broadcast + raise + end end end.each(&:join) }.to raise_error(ActiveRecord::ConnectionTimeoutError)

https://github.com/abicky/activerecord-debug_errors/pull/3/files#r1615209397

https://github.com/abicky/activerecord-debug_errors/pull/3/files#r1615204591

abicky

Again, thank you for your contribution! Your investigation helped a lot.

I wonder if it's okay to remove 2.6 to 3.0 support since they are already EOL.

I think we can stop supporting the versions but it might be better not to deal with the issue on this PR.

abicky · 2024-05-26T15:56:34Z

I've released v0.1.3.

ohbarye · 2024-05-27T00:40:15Z

I think we can stop supporting the versions but it might be better not to deal with the issue on this PR.

Agree.

Thanks for your quick review and release too!

ohbarye added 2 commits May 26, 2024 17:21

Run test against Ruby 3.2, 3.3

08b8284

ohbarye added 3 commits May 26, 2024 19:06

Retrieve execute hook for 6.0 and 6.1

6214941

ohbarye force-pushed the run-test-against-newer-rubies branch from bf0b8b7 to 6214941 Compare May 26, 2024 11:22

ohbarye added 2 commits May 26, 2024 20:29

Add test matrix: Rails minor versions should be tested

ca6d8e4

Retrieve raw_execute hook for 7.0

9cc944f

ohbarye changed the title ~~Run test against Ruby 3.2, 3.3~~ Support Ruby 3.2, 3.3 and Rails 7.1 May 26, 2024

ohbarye commented May 26, 2024

View reviewed changes

abicky reviewed May 26, 2024

View reviewed changes

ohbarye added 2 commits May 26, 2024 23:59

Use more stable way to cause ConnectionTimeoutError

d2a4654

https://github.com/abicky/activerecord-debug_errors/pull/3/files#r1615209397

Retrieve the original prepend way so that it works even in 7.1

6dbd03f

https://github.com/abicky/activerecord-debug_errors/pull/3/files#r1615204591

abicky approved these changes May 26, 2024

View reviewed changes

abicky merged commit 78fe080 into abicky:master May 26, 2024
22 checks passed

ohbarye deleted the run-test-against-newer-rubies branch May 27, 2024 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Ruby 3.2, 3.3 and Rails 7.1 #3

Support Ruby 3.2, 3.3 and Rails 7.1 #3

ohbarye commented May 26, 2024 •

edited

Loading

ohbarye commented May 26, 2024

ohbarye commented May 26, 2024

ohbarye May 26, 2024

abicky May 26, 2024 •

edited

Loading

ohbarye May 26, 2024

abicky May 26, 2024

ohbarye May 27, 2024

ohbarye May 26, 2024

abicky May 26, 2024 •

edited

Loading

ohbarye May 26, 2024

ohbarye May 26, 2024

abicky left a comment

abicky May 26, 2024 •

edited

Loading

abicky May 26, 2024 •

edited

Loading

abicky left a comment

abicky commented May 26, 2024

ohbarye commented May 27, 2024

Support Ruby 3.2, 3.3 and Rails 7.1 #3

Support Ruby 3.2, 3.3 and Rails 7.1 #3

Conversation

ohbarye commented May 26, 2024 • edited Loading

Changes

Thoughts

ohbarye commented May 26, 2024

ohbarye commented May 26, 2024

ohbarye May 26, 2024

Choose a reason for hiding this comment

abicky May 26, 2024 • edited Loading

Choose a reason for hiding this comment

ohbarye May 26, 2024

Choose a reason for hiding this comment

abicky May 26, 2024

Choose a reason for hiding this comment

ohbarye May 27, 2024

Choose a reason for hiding this comment

ohbarye May 26, 2024

Choose a reason for hiding this comment

abicky May 26, 2024 • edited Loading

Choose a reason for hiding this comment

ohbarye May 26, 2024

Choose a reason for hiding this comment

ohbarye May 26, 2024

Choose a reason for hiding this comment

abicky left a comment

Choose a reason for hiding this comment

abicky May 26, 2024 • edited Loading

Choose a reason for hiding this comment

abicky May 26, 2024 • edited Loading

Choose a reason for hiding this comment

abicky left a comment

Choose a reason for hiding this comment

abicky commented May 26, 2024

ohbarye commented May 27, 2024

ohbarye commented May 26, 2024 •

edited

Loading

abicky May 26, 2024 •

edited

Loading

abicky May 26, 2024 •

edited

Loading

abicky May 26, 2024 •

edited

Loading

abicky May 26, 2024 •

edited

Loading